Automated Test: implement-device-limits #344
Conversation
* Anonymous: Add device limiter * break auth if limit reached * fix typo * refactored const to make it clearer with expiration * anon device limit for config --------- Co-authored-by: Eric Leijonmarck <eric.leijonmarck@gmail.com>
📝 WalkthroughWalkthroughThis PR introduces anonymous device limit enforcement across Grafana by adding configuration properties to TypeScript and Go backends, implementing per-device limit logic in the anonymous device store with a new error type, updating frontend settings to expose the configured limit, and refactoring service dependency injection to initialize the store with the configured device limit. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/services/anonymous/anonimpl/impl.go (1)
100-115:⚠️ Potential issue | 🟡 MinorPre-existing bug:
untagDevicelogs the wrong error variable.Line 113 logs
err(the function parameter, which isnilat this point due to the early return on line 102-104) instead oferrD(the actual deletion error). While not introduced in this PR, it's adjacent to the changed code and worth fixing.🐛 Proposed fix
errD := a.anonStore.DeleteDevice(ctx, deviceID) if errD != nil { - a.log.Debug("Failed to untag device", "error", err) + a.log.Debug("Failed to untag device", "error", errD) }
🤖 Fix all issues with AI agents
In `@pkg/api/dtos/frontend_settings.go`:
- Line 195: Change the DTO field AnonymousDeviceLimit from a non-pointer int64
to a pointer (*int64) and add the `omitempty` json tag so it serializes as
absent when unset (matches the TS type `number | undefined`); update any
constructors/initializers or places that populate the struct (e.g., where
FrontendSettings or its factory is created) to set nil when there is no
configured limit and to take the address of an int64 value when you need to
provide one, and audit any consumers (serialization, validation, defaults) of
AnonymousDeviceLimit to handle a nil pointer appropriately.
In `@pkg/services/anonymous/anonimpl/anonstore/database.go`:
- Around line 105-118: The CountDevices check in CreateOrUpdateDevice is a
TOCTOU race: CountDevices and the subsequent insert/update (via updateDevice)
run in separate DB sessions so concurrent callers can exceed deviceLimit; fix by
making the check-and-insert atomic—either perform the count and insert/update
inside a single database transaction in CreateOrUpdateDevice (so use the same DB
transaction/session and rollback on conflict) or enforce the limit with a
DB-level constraint (unique/conditional constraint or a counter row) and handle
the constraint error in CreateOrUpdateDevice; if you intentionally want a
best-effort limit, add a clear comment in CreateOrUpdateDevice mentioning that
deviceLimit is advisory and not strictly enforced.
In `@pkg/services/anonymous/anonimpl/client.go`:
- Around line 44-50: Synchronous call to anonDeviceService.TagDevice on the
request path (in anonimpl client code) causes per-request DB latency; either
validate this is acceptable under expected load or implement a short-circuit
cache/in-memory counter to avoid calling TagDevice for the common “well below
limit” case and only invoke TagDevice when the in-memory count nears the limit;
specifically add an in-memory device-count cache keyed by anonymous identity
(with TTL and atomic increments/decrements) used by the authentication flow to
skip TagDevice when safely below threshold, and fall back to calling
anonDeviceService.TagDevice (and honoring anonstore.ErrDeviceLimitReached) when
cache indicates a possible limit breach.
🧹 Nitpick comments (4)
packages/grafana-runtime/src/config.ts (1)
97-97: Missing explicit type annotation — inferred type isundefinedinstead ofnumber | undefined.Without an explicit annotation, TypeScript infers this property as type
undefined, so code accessingconfig.anonymousDeviceLimitvia aGrafanaBootConfigreference won't seenumberas a valid type. This is consistent with other fields in this class (e.g.,googleAnalyticsId,rudderstackWriteKey), so it's a pre-existing pattern, but worth fixing here for type safety.Suggested fix
- anonymousDeviceLimit = undefined; + anonymousDeviceLimit: number | undefined = undefined;pkg/setting/setting.go (1)
1655-1655: Consider adding validation fordevice_limitto document expected behavior.While the code safely handles negative values (treating them as 0 to disable the limit via the
if s.deviceLimit > 0check), adding explicit validation at config load time would clarify intent. Optional, but defensible.Suggested fix
cfg.AnonymousDeviceLimit = anonSection.Key("device_limit").MustInt64(0) + if cfg.AnonymousDeviceLimit < 0 { + cfg.AnonymousDeviceLimit = 0 + }pkg/services/anonymous/anonimpl/anonstore/database.go (1)
18-18: Useerrors.Newinstead offmt.Errorffor sentinel errors.
ErrDeviceLimitReachedhas no format arguments, soerrors.Newis the idiomatic choice and avoids unnecessary formatting overhead.♻️ Proposed fix
+import "errors" + -var ErrDeviceLimitReached = fmt.Errorf("device limit reached") +var ErrDeviceLimitReached = errors.New("device limit reached")pkg/services/anonymous/anonimpl/anonstore/database_test.go (1)
51-70: Test covers the basic limit enforcement path well.Consider adding a complementary test case that verifies an existing device (same
DeviceID) can still be updated when the limit is reached. This would exercise theupdateDevicehappy path whererowsAffected > 0.
| @@ -192,6 +192,7 @@ type FrontendSettingsDTO struct { | |||
|
|
|||
| FeatureToggles map[string]bool `json:"featureToggles"` | |||
| AnonymousEnabled bool `json:"anonymousEnabled"` | |||
| AnonymousDeviceLimit int64 `json:"anonymousDeviceLimit"` | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how anonymousDeviceLimit is consumed on the frontend side
rg -n --type=ts --type=tsx -C3 'anonymousDeviceLimit' -g '!node_modules'Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Check how the value is set on the Go side to understand the semantics of 0
rg -n -C3 'AnonymousDeviceLimit' --type=goRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 2989
🏁 Script executed:
#!/bin/bash
# Search for anonymousDeviceLimit in TypeScript/JavaScript files
rg -n 'anonymousDeviceLimit' --type-add 'ts:*.{ts,tsx}' --type tsRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 253
🏁 Script executed:
#!/bin/bash
# Also check if there's documentation or comments about what 0 means for device limit
rg -n -B3 -A3 'device_limit' --type=goRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 624
🏁 Script executed:
#!/bin/bash
# Find how anonymousDeviceLimit is used in frontend logic
rg -n 'anonymousDeviceLimit' -A5 -B2 --type=ts | head -100Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1354
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about what the device limit value means
rg -n -B5 -A5 'device.limit|device_limit' --type=go | head -80Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 2779
🏁 Script executed:
#!/bin/bash
# Find all usages of anonymousDeviceLimit in the frontend
rg -n 'anonymousDeviceLimit' -B3 -A10 packages/Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 2258
🏁 Script executed:
#!/bin/bash
# Check the anonstore logic more carefully to confirm the semantics of deviceLimit
rg -n 'deviceLimit' pkg/services/anonymous/anonimpl/anonstore/database.go -B2 -A2Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 699
🏁 Script executed:
#!/bin/bash
# Broader search for any reference to anonymousDeviceLimit
git -C . grep -n "anonymousDeviceLimit" -- "packages/" "public/" "src/" 2>/dev/null | grep -v "\.map:" || echo "No matches found"Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 253
🏁 Script executed:
#!/bin/bash
# Check if there are any computed properties or indirect references using the config object
rg -n "config\.anon|getConfig\(\)\.anon" --type=tsRepository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 219
Consider using *int64 with omitempty to align with the TypeScript type.
The Go DTO field AnonymousDeviceLimit is int64 without omitempty, which means it always serializes to JSON (as 0 when unconfigured). However, the TypeScript interface declares anonymousDeviceLimit: number | undefined, and the frontend config defaults to undefined. This creates a minor type inconsistency: when no limit is configured, the backend sends {"anonymousDeviceLimit": 0}, which deserializes to 0 rather than undefined.
While the field is currently unused on the frontend, using *int64 with omitempty would clarify intent and prevent issues if the value is consumed later:
Suggested fix
- AnonymousDeviceLimit int64 `json:"anonymousDeviceLimit"`
+ AnonymousDeviceLimit *int64 `json:"anonymousDeviceLimit,omitempty"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AnonymousDeviceLimit int64 `json:"anonymousDeviceLimit"` | |
| AnonymousDeviceLimit *int64 `json:"anonymousDeviceLimit,omitempty"` |
🤖 Prompt for AI Agents
In `@pkg/api/dtos/frontend_settings.go` at line 195, Change the DTO field
AnonymousDeviceLimit from a non-pointer int64 to a pointer (*int64) and add the
`omitempty` json tag so it serializes as absent when unset (matches the TS type
`number | undefined`); update any constructors/initializers or places that
populate the struct (e.g., where FrontendSettings or its factory is created) to
set nil when there is no configured limit and to take the address of an int64
value when you need to provide one, and audit any consumers (serialization,
validation, defaults) of AnonymousDeviceLimit to handle a nil pointer
appropriately.
| func (s *AnonDBStore) CreateOrUpdateDevice(ctx context.Context, device *Device) error { | ||
| var query string | ||
|
|
||
| // if device limit is reached, only update devices | ||
| if s.deviceLimit > 0 { | ||
| count, err := s.CountDevices(ctx, time.Now().UTC().Add(-anonymousDeviceExpiration), time.Now().UTC().Add(time.Minute)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if count >= s.deviceLimit { | ||
| return s.updateDevice(ctx, device) | ||
| } | ||
| } |
There was a problem hiding this comment.
TOCTOU race: count check + insert are not atomic.
CountDevices and the subsequent insert/update run in separate DB sessions. Under concurrent requests, multiple goroutines can pass the count check simultaneously and all proceed to insert, exceeding the configured device limit.
If strict enforcement is required, consider wrapping the count + insert in a single transaction, or using a database-level constraint. If this is intended as a soft/best-effort limit, a brief comment documenting that would help future readers.
🤖 Prompt for AI Agents
In `@pkg/services/anonymous/anonimpl/anonstore/database.go` around lines 105 -
118, The CountDevices check in CreateOrUpdateDevice is a TOCTOU race:
CountDevices and the subsequent insert/update (via updateDevice) run in separate
DB sessions so concurrent callers can exceed deviceLimit; fix by making the
check-and-insert atomic—either perform the count and insert/update inside a
single database transaction in CreateOrUpdateDevice (so use the same DB
transaction/session and rollback on conflict) or enforce the limit with a
DB-level constraint (unique/conditional constraint or a counter row) and handle
the constraint error in CreateOrUpdateDevice; if you intentionally want a
best-effort limit, add a clear comment in CreateOrUpdateDevice mentioning that
deviceLimit is advisory and not strictly enforced.
| if err := a.anonDeviceService.TagDevice(ctx, httpReqCopy, anonymous.AnonDeviceUI); err != nil { | ||
| if errors.Is(err, anonstore.ErrDeviceLimitReached) { | ||
| return nil, err | ||
| } | ||
| }() | ||
|
|
||
| a.log.Warn("Failed to tag anonymous session", "error", err) | ||
| } |
There was a problem hiding this comment.
Synchronous TagDevice adds latency to every anonymous request — verify this is acceptable.
Previously, TagDevice ran asynchronously in a goroutine with a timeout. Now it's synchronous on the request path, meaning every anonymous authentication blocks until the DB round-trip completes. This is necessary for correct device-limit enforcement (you need the result before deciding whether to allow the request), but it adds latency to all anonymous requests, not just those hitting the limit.
Consider whether a caching layer or in-memory check could short-circuit the common case (well below limit) to reduce per-request overhead, or whether the current approach is acceptable given expected traffic patterns.
The error handling logic itself is correct: ErrDeviceLimitReached blocks authentication while other errors are logged and authentication proceeds.
🤖 Prompt for AI Agents
In `@pkg/services/anonymous/anonimpl/client.go` around lines 44 - 50, Synchronous
call to anonDeviceService.TagDevice on the request path (in anonimpl client
code) causes per-request DB latency; either validate this is acceptable under
expected load or implement a short-circuit cache/in-memory counter to avoid
calling TagDevice for the common “well below limit” case and only invoke
TagDevice when the in-memory count nears the limit; specifically add an
in-memory device-count cache keyed by anonymous identity (with TTL and atomic
increments/decrements) used by the authentication flow to skip TagDevice when
safely below threshold, and fall back to calling anonDeviceService.TagDevice
(and honoring anonstore.ErrDeviceLimitReached) when cache indicates a possible
limit breach.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes